Skip to content

fix(service-storage): 无作用域的 sys_attachment 多行删除改为失败关闭 (#4757) - #4780

Merged
os-zhuang merged 1 commit into
mainfrom
claude/issue-4757-attachment-unscoped-delete
Aug 3, 2026
Merged

fix(service-storage): 无作用域的 sys_attachment 多行删除改为失败关闭 (#4757)#4780
os-zhuang merged 1 commit into
mainfrom
claude/issue-4757-attachment-unscoped-delete

Conversation

@os-zhuang

Copy link
Copy Markdown
Contributor

Fixes #4757

问题

packages/services/service-storage/src/attachment-access-hooks.tsbeforeDelete 守卫用两条路径解析「这次删除命中了哪些行」——按 input.id,或按 input.options.where——然后 if (!rows.length) return

既没有 id 也没有 where 的删除两条分支都不进,rows 保持为空,守卫返回放行。但这不是「没命中任何行」,而是从来没有查询过 —— 两者是不同的判断,把后者当前者就是典型的 fail open。

engine 那边把同一次调用当作全表批删:packages/objectql/src/engine.ts 的 delete 路径在没有标量 id 时把 AST 播种成 { object }(没有 where),再交给 driver.deleteMany。于是 ql.delete('sys_attachment', { multi: true }) 会清空整张附件表,而记录级守卫恰好授权了零行

下面两层都接不住:

  • plugin-sharing —— buildWriteFilter 对没有 owner 字段的对象返回 null,而 sys_attachment 的来源列是 uploaded_by 不是 owner_id,所以 AST 上不会被合成任何行级作用域谓词。
  • plugin-security —— member_default 基线不带 allowDelete(ADR-0090 D5),普通成员会先被 RBAC 拒掉;但只要 app 发了一份带 sys_attachment 删除位的领域授权(附件面板本来就需要,attachmentsFixtureatt_attachment_manager 正是这么建模的),就能通过 RBAC 直接落到这条没有守卫的路径上。

改动

按 issue 的建议失败关闭,姿势照抄 #4630sys_comment 立的 resolveTargetRows:无 id 且无 where ⇒ 403 ATTACHMENT_DELETE_DENIED,而不是穿过空 rows 短路。

合法路径完全未动,这一点专门写了回归用例:

  • 按 id 删除 —— 照旧逐行做 uploader-or-parent-editor 判定;
  • where 的多行删除 —— 照旧解析匹配行集合(1000 行上界不变)并逐行判定;
  • where: {} —— 它匹配所有行,但是一次真实查询,所以仍然走「逐行授权」而不是「拒绝」:每一行都过才放行,有一行不过就 403;
  • 谓词跑了但一行都没匹配到 —— 仍然放行(这才是真正的「没有东西需要授权」);
  • isSystem 与无 session 的程序化调用 —— 旁路不变。

.changeset/attachment-unscoped-multi-delete.md 里写了迁移说明:如果确有清空附件表的需求,传谓词({ multi: true, where: {} },逐行授权)或者在 system context 下做。

测试

packages/services/service-storage/src/attachment-access-hooks.test.ts 在既有 beforeDelete 用例旁边新增一组 #4757 用例。先验证过它们在未修复的源码上会红(回退 src、保留 test:5 failed | 16 passed),打上修复后:

pnpm --filter @objectstack/service-storage exec vitest run --maxWorkers=2
 Test Files  19 passed (19)
      Tests  260 passed (260)

类型侧:该包在 scripts/check-type-check-coverage.mjs 里是 DEBT 条目(42 errors,没有 typecheck 脚本),tsc --noEmit 实测仍是 42,本次改动的两个文件零错误;pnpm --filter @objectstack/service-storage build(含 DTS)通过。

同形状排查(issue 末尾那句)

按要求全仓扫了一遍「没解析到行 ⇒ 放行」这个形状(beforeDeletebeforeUpdate 一类守卫,检索 registerHook('before*'input?.idoptions?.whereif (!id) returnif (!rows.length) 等)。发现的其它位置都没有在本 PR 里修,按 Prime Directive #10 另立了未认领 issue:

以下位置形状相同但不是授权守卫,后果是数据/可观测性而非越权,且多数在注释里已声明为已知边界,只在 #4778 正文里记录、未单独立 issue:plugin-auditcaptureBefore(批量写不留审计快照,注释写明「too costly」)、plugin-webhooks/plugin-email/plugin-sharing 三处 provenance 盖章(注释均写明 known boundary)、plugin-sharingprimary-bu-projection beforeDelete(投影 best-effort,靠下次写入或 boot backfill 自愈)。

明确排除的:plugin-authidentity-write-guard —— 它的 beforeDelete 是无条件拒绝、beforeUpdate 走字段白名单,不依赖行解析,不属于这个形状。

范围

改动只落在 packages/services/service-storage/。issue 正文引用的 packages/objectql/src/engine.ts delete 路径只作为背景阅读,未改动(本轮 #4769 正在改 objectql);plugin-authservice-automationplugin-approvals 也只读未改。


Generated by Claude Code

…nt (#4757)

The `beforeDelete` gate in `installAttachmentAccessHooks` resolved the rows a
delete matches by `input.id` or by `input.options.where`, then short-circuited
on `if (!rows.length) return`. A delete with NEITHER took neither branch, so
the gate returned allow — while the engine seeded `{ object }` as the delete
AST and handed that to `driver.deleteMany`, emptying the table. "Nothing to
authorize" and "nothing was ever queried" are different verdicts.

Fail closed on that shape instead: no id and no `where` throws 403
`ATTACHMENT_DELETE_DENIED`, the posture #4630 gave `sys_comment` in
`resolveTargetRows`. Scoped paths are untouched — an id-bound delete, a
`where`-bound multi-delete and `where: {}` all still resolve their rows and
authorize each one, and a query that genuinely matches nothing still passes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Br2xsJsczFsTR9bvbh2Ny
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 3, 2026 6:14am

Request Review

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-storage.

4 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/api/plugin-endpoints.mdx (via @objectstack/service-storage)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/service-storage)
  • content/docs/plugins/packages.mdx (via @objectstack/service-storage)
  • content/docs/releases/implementation-status.mdx (via @objectstack/service-storage)

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

@os-zhuang
os-zhuang marked this pull request as ready for review August 3, 2026 06:31
@os-zhuang
os-zhuang added this pull request to the merge queue Aug 3, 2026
Merged via the queue into main with commit 941dec4 Aug 3, 2026
21 checks passed
@os-zhuang
os-zhuang deleted the claude/issue-4757-attachment-unscoped-delete branch August 3, 2026 06:43
os-zhuang pushed a commit that referenced this pull request Aug 3, 2026
合并 #4768(C17)/ #4786 / #4780 / #4783(#4634,31 位能力位退役)后重生成。

按 #4535 §7 与「rebase/merge 静默回退」纪律处理三张 ratchet:
`dual-source-exports.baseline.json` / `authorable-surface.json` /
`json-schema.manifest.json`(+ `api-surface.json`)一律 `git checkout
origin/main --` 取 main 版本后全量重跑生成器,只重施本 PR 的一处改动。
其中 json-schema.manifest.json 与 authorable-surface.json 归 os-regen
merge driver 管、合并不产生冲突标记,最易静默回退,故以 gen:schema 实跑
输出为准。

逐项确认他人蓄意变更未被回滚(regen 后实测):
- #4783 `data/DriverCapabilities:*` 31 行 [RETIRED] + 3 行存活 —— 均在
- C10 `system/EnvironmentArtifact*` 删除 —— 仍为 0
- C17 `studio/ActionLocation` → `studio/ActionContributionLocation` —— 旧 0 新 1
- 本 PR `kernel/PackageDependency` → `kernel/ResolvedPackageDependency` —— 旧 0 新 1

`renamed-defs.ts` 冲突为两条独立改名条目并存(C17 与本簇),按时序保留两条。

dual-source 基线 2 → **0**(entries: []),#4535 第二批收官、双源账目归零。

Claude-Session: https://claude.ai/code/session_0176qgxgCXTJCUv4YFLtusP9

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

2 participants